-
Notifications
You must be signed in to change notification settings - Fork 688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add extra error logging to auth response errors #6140
Conversation
🦋 Changeset detectedLatest commit: a323fbe The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
de7e2c1
to
3f8f9b4
Compare
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-wrangler-6140 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6140/npm-package-wrangler-6140 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-wrangler-6140 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-create-cloudflare-6140 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-cloudflare-kv-asset-handler-6140 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-miniflare-6140 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-cloudflare-pages-shared-6140 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9681349168/npm-package-cloudflare-vitest-pool-workers-6140 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One very minor comment. Otherwise, looks good 👍
@@ -1249,3 +1252,27 @@ async function fetchAuthToken(body: URLSearchParams) { | |||
headers, | |||
}); | |||
} | |||
|
|||
async function getJSONFromResponse(response: Response) { | |||
const text = await response.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, but if you passed in the text string to this function, it would reduce its responsibilities:
async function getJSONFromText<T>(text: string): T {
try ...
}
...
const json = getJSONFromText<TokenResponse>(await response.text());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIght, but then we would have to extract the text on each of the three calls, which seems a bit repetitive; but more importantly, the logs grab the status from the response so that would need to be passed in too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we might also want to use the status code as part of the analysis of what went wrong with the response (e.g. I think that the Bot Management failure would have a 403 status).
We're going to hold off landing this given our limited deployment window this week. But if you are experiencing issues as linked in the description above, you could try this version of Wrangler via:
Or perhaps installing it into your project (and using it as normal) via:
|
3f8f9b4
to
a323fbe
Compare
What this PR solves / how to test
May help diagnosing issues like #3672, #5542 and https://jira.cfdata.org/browse/CUSTESC-42645
Author has addressed the following